perf(jsonrpc): join batch response bytes instead of re-encoding - #3998

Claude finished @NazariiDenha's task in 3m 5s —— View job

Review: perf(jsonrpc): join batch response bytes instead of re-encoding

• Gather context (diff, CLAUDE.md, related code)
• Understand the request (code review)
• Review jsonrpc/server.go changes
• Review jsonrpc/server_test.go changes
• Post findings

Summary: This is a clean, low-risk optimization. concatBatchResponses replaces json.Marshal(responses) with direct byte concatenation, avoiding a redundant compact()/re-escape pass over already-marshaled JSON. Since every element in responses originates from json.Marshal(response) in addResponse (server.go:450), it's guaranteed to already be compact, HTML-escaped JSON — so skipping the second compact pass is safe and produces byte-identical output. Capacity pre-allocation (size := len(responses) + 1 + Σlen(response)) correctly matches the existing totalBytes accounting used for the batch-size limit, so no reallocation occurs during the build.

The new test TestBatchArrayIsByteIdenticalToJSONMarshal is a good addition — it specifically exercises the cases that would break if HTML-escaping/Unicode-line-separator handling diverged (<script>, &&, U+2028/U+2029, quotes/backslashes), by round-tripping the server's actual output through json.Marshal and asserting byte equality.

Nit (posted inline): minor grammar fix in the new doc comment, plus a suggestion to note the implicit invariant that concatBatchResponses depends on its inputs always being compact/pre-escaped json.RawMessage — not a bug today, just worth flagging for future maintainers who might reuse this helper with differently-sourced raw messages.

No blockers or important issues found.

Note: I wasn't able to run go test ./jsonrpc/... or golangci-lint in this sandbox (tool execution beyond read-only git commands required approval that wasn't available non-interactively). Review is based on static analysis of the diff and knowledge of encoding/json's RawMessage/compact semantics — you may want to confirm CI passes for the new test.

PR Reviewer Guide 🔍

(Review updated until commit 89de865)

Here are some key observations to aid the review process:

Claude finished @NazariiDenha's task in 1m 1s —— View job

Review: perf(jsonrpc): join batch response bytes instead of re-encoding

• Gather context (diff against origin/main, addResponse/handleBatchRequest in jsonrpc/server.go)
• Review correctness of concatBatchResponses
• Review capacity pre-allocation math
• Review error-handling change (dropped err from json.Marshal)
• Review new test TestBatchArrayIsByteIdenticalToJSONMarshal
• Post summary

Summary

No blockers or important issues. This is a correct, low-risk hot-path optimization.

Correctness:

• Every element in responses (jsonrpc/server.go:460) is produced by json.Marshal(response) inside addResponse, so each json.RawMessage is guaranteed compact and HTML-escaped before concatBatchResponses runs. That's exactly the invariant json.Marshal([]json.RawMessage)'s internal compact() pass would have been enforcing, so skipping it and just joining bytes is safe and produces byte-identical output.
• Capacity math (jsonrpc/server.go:570-573) is exact: for N elements it's N+1 (brackets + separators) plus Σlen(response), matching the existing totalBytes accounting used for the batch-size limit — no reallocation during the build.
• Dropping the error return (previously from json.Marshal) is safe: the only way that marshal could fail is if a RawMessage held invalid JSON, which can't happen here since every entry already round-tripped through json.Marshal successfully in addResponse.
• No changes to the concurrency/aggregation logic (wg.Wait(), mutex-guarded addResponse) — this only touches how the already-collected responses are serialized, so no new race conditions.

Tests:

• TestBatchArrayIsByteIdenticalToJSONMarshal (jsonrpc/server_test.go:1054) is a solid regression test — it specifically exercises <script>, &&, U+2028/U+2029, and quote/backslash payloads (the cases that would diverge if HTML-escaping ever differed between the per-element and batch-level marshal), and asserts byte-for-byte equality against json.Marshal.

Nit (already flagged by another review pass on this PR): the doc comment on concatBatchResponses has a small grammar issue and could note explicitly that correctness depends on inputs always being pre-compacted/escaped json.RawMessage — worth keeping in mind if this helper is ever reused with differently-sourced input, but not a functional issue today.

No security, panic, or resource-leak concerns identified in this diff.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.18%. Comparing base (f103674) to head (cc3a4ac).

@@            Coverage Diff             @@
##             main    #3998      +/-   ##
==========================================
- Coverage   79.29%   79.18%   -0.12%     
==========================================
  Files         465      465              
  Lines       35785    35794       +9     
==========================================
- Hits        28377    28344      -33     
- Misses       7399     7441      +42     
  Partials        9        9              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Persistent review updated to latest commit 89de865

Claude finished @NazariiDenha's task in 2m 50s —— View job

Review: perf(jsonrpc): join batch response bytes instead of re-encoding

• Gather context (diff vs origin/main, handleBatchRequest/addResponse in jsonrpc/server.go)
• Review concatBatchResponses correctness and capacity math
• Review dropped error return from batch marshal
• Review new test TestBatchArrayIsByteIdenticalToJSONMarshal
• Build (go build ./jsonrpc/...) and go vet ./jsonrpc/...
• Post summary

Verdict: clean, no blockers or important issues.

Correctness

• Every element of responses (jsonrpc/server.go:460) is produced by json.Marshal(response) inside addResponse, so each json.RawMessage is guaranteed compact + HTML-escaped before concatBatchResponses (jsonrpc/server.go:573) runs. That's exactly the invariant encoding/json's internal compact() pass enforced previously, so joining bytes directly is safe and byte-identical.
• Capacity pre-allocation is exact: size := len(responses) + 1 + Σlen(response) accounts for the 2 brackets + (n-1) separators = n+1 extra bytes, matching the existing totalBytes accounting used for the batch-size limit (server.go:446,459). No reallocation during the build.
• Dropping the error return is safe — the only way the old json.Marshal(responses) could fail is an invalid RawMessage, which can't happen since every entry already round-tripped through a successful json.Marshal in addResponse.
• The empty-batch guard (len(responses) == 0 → nil, finalHeaders, nil, server.go:534-536) still runs before concatBatchResponses, so it's never called with zero elements — no div-by-zero/edge case there.
• No changes to the concurrency model (mutex-guarded addResponse, wg.Wait() before use) — this only changes how already-collected responses are serialized.
• Confirmed locally: go build ./jsonrpc/... and go vet ./jsonrpc/... both pass clean. (Sandbox didn't allow running go test/golangci-lint non-interactively — worth confirming CI is green.)

Tests

• TestBatchArrayIsByteIdenticalToJSONMarshal (jsonrpc/server_test.go:1054) is a solid regression test: it exercises <script>/&& (HTML-escape chars), U+2028/U+2029 (line separators encoding/json normally escapes), quotes/backslashes, and an empty payload — then asserts the server's actual batch output is byte-identical to json.Marshal of the decoded responses. Good coverage of exactly the cases that would silently diverge if this optimization were wrong.

Nit (already flagged on this PR, still unresolved)

• jsonrpc/server.go:570-572 doc comment has a grammar slip ("elements that already valid JSON") and could spell out the invariant that correctness depends on inputs always being pre-compacted/escaped json.RawMessage (as produced by addResponse). Not a functional issue today — just worth tightening before this helper is reused elsewhere with differently-sourced input.

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

// concatBatchResponses builds the JSON array from elements that already valid
// JSON, so it joins bytes instead of re-encoding. json.Marshal would run every
// byte through compact() again, which is costly
func concatBatchResponses(responses []json.RawMessage) []byte {
	size := len(responses) + 1
	for _, response := range responses {
		size += len(response)
	}

	result := make([]byte, 0, size)
	result = append(result, '[')
	for i, response := range responses {
		if i > 0 {
			result = append(result, ',')
		}
		result = append(result, response...)
	}

	return append(result, ']')
}

Claude finished @NazariiDenha's task in 2m 39s —— View job

Review: perf(jsonrpc): join batch response bytes instead of re-encoding

• Gather context (diff vs origin/main, handleBatchRequest/addResponse in jsonrpc/server.go)
• Verify build/vet (go build ./jsonrpc/..., go vet ./jsonrpc/... — both pass, confirms server_test.go also compiles)
• Post final review summary

Verdict: clean, no blockers.

This code has already been reviewed multiple times on this thread; re-checked against the current HEAD (cc3a4ac, same diff as previously reviewed) and nothing new stands out.

Correctness (confirmed again):

• concatBatchResponses (jsonrpc/server.go:573) only ever receives elements appended by addResponse (server.go:449-461), which appends to responses only after json.Marshal(response) succeeds — a failed marshal is logged and the element is dropped, never added. So by construction every json.RawMessage passed in is already valid, compact, HTML-escaped JSON. The "Silent Error Swallowing" concern raised by the automated PR-Agent pass (that malformed elements could produce a broken array) doesn't hold — there's no code path that reaches concatBatchResponses with an unvalidated/invalid element.
• Capacity math (size := len(responses) + 1 + Σlen(response)) is exact (n+1 for brackets/separators), matching the existing totalBytes accounting — no reallocation during the build.
• No changes to concurrency/aggregation (mutex-guarded addResponse, wg.Wait() before use).

Tests: TestBatchArrayIsByteIdenticalToJSONMarshal (server_test.go:1054) is a solid regression test, covering HTML-escape chars, U+2028/U+2029, quotes/backslashes, and an empty payload, asserting byte-identical output vs json.Marshal.

Nit (already flagged inline, unaddressed, non-blocking): the doc comment grammar ("elements that already valid JSON") on server.go:570-572 is still unfixed — small polish item, not worth blocking on.

Design discussion (already addressed by author): infrmtcs's suggestion to wrap this in a custom RawSlice type with MarshalJSON was reasonably countered by the author — a custom MarshalJSON still goes through json.Marshal's reflection/interface-dispatch overhead, which is exactly what this PR avoids.

Note: sandbox permissions allow go build/go vet but not go test non-interactively in this run — build and vet both pass clean; recommend confirming CI test run is green before merge.

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

func concatBatchResponses(responses []json.RawMessage) []byte {
	size := len(responses) + 1
	for _, response := range responses {
		size += len(response)
	}

	result := make([]byte, 0, size)
	result = append(result, '[')
	for i, response := range responses {
		if i > 0 {
			result = append(result, ',')
		}
		result = append(result, response...)
	}

	return append(result, ']')
}